-
Notifications
You must be signed in to change notification settings - Fork 285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Quarto articles #2015
Support Quarto articles #2015
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be super helpful! I suggested a few changes, but this is well on its way! You'll also need to resolve a merge conflict in the NEWS.md (I couldn't fix that piece).
@@ -13,40 +13,60 @@ | |||
#' @param name Base for file name to use for new vignette. Should consist only | |||
#' of numbers, letters, `_` and `-`. Lower case is recommended. | |||
#' @param title The title of the vignette. | |||
#' @param type One of `"quarto"` or `"rmarkdown"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#' @param type One of `"quarto"` or `"rmarkdown"` | |
#' @param builder One of `"quarto"` or `"rmarkdown"` |
type
is a little over-generic and could collide with another param. Let's use the more-specific builder
(to match "VignetteBuilder"). If you go with this change, it needs to be changed throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed that type
may not be ideal. If we were to go with builder, the arguments should be updated to "knitr" instead of "rmarkdown"?
Maybe ext = c("qmd", "rmd")
would work too.
Maybe there should be an option
options(usethis.vignette = "quarto")
to avoid specifying it everytime?
I can imagine myself deciding to stick with a single type, and not have to specify it everytime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like ext
, and an option would be great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for your helpful comments! I will get this in a better shape
Co-authored-by: Jon Harmon <[email protected]>
Thanks to both @olivroy and @jonthegeek! I won't be able to complete thinking about this and merge this today, but I will definitely do so soon. And we're clearly very close! |
@jennybc not sure when you'll have cycles next; but, if you could hint at any changes required to take the Quarto vignette action across the finish line, I'm happy to help. |
Gentle bump on adding Quarto vignettes into |
I will come back to this soon, as I need to release usethis for CRAN reasons. |
Now that upcoming pkgdown version supports them!
fixes #1997
With the release of pkgdown 2.1, I think it is worth adding to usethis!
I set
overwrite = FALSE
to setting the VignetteBuilder field. Not sure how having quarto and rmarkdown vignettes will work. I can do more testing after review!Would need the following too in Rbuildignore https://github.com/r-lib/pkgdown/pull/2656/files